Skip to content

extend_mode_normalized and extend_mode #974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 29, 2025

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented May 7, 2025

Current extend_mode is renamed to extend_mode_normalized (because it require normalized coordinates). To avoid rounding errors we introduce extend_mode that relaxes normalization requirement. Currently only EXTEND_PAD is implemented differently, others fallback to extend_mode_normalized for now.

partial fix #972

@sagudev sagudev force-pushed the extend_mode_norm branch from 1465e2d to 5059273 Compare May 7, 2025 12:29
@sagudev sagudev marked this pull request as ready for review May 7, 2025 12:37
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought on testing. Once that's addressed, I'll be happy to land. I don't think this is going to be meaningfully reviewed by anyone else anytime soon.

@sagudev
Copy link
Contributor Author

sagudev commented May 20, 2025

It works on my local ubuntu machine but not in CI. Is this the case for #[cfg_attr(skip_gpu_tests, ignore)]?

@DJMcNab
Copy link
Member

DJMcNab commented May 21, 2025

The windows failure certainly would be caused by the lack of GPU support for those runners. I'm not sure what the ubuntu failure is. Unfortunately, #892 means that the "current version" uploading of the snapshots is currently broken. I'll open a PR to fix that today.

@DJMcNab
Copy link
Member

DJMcNab commented May 26, 2025

And if you can rebase #1018, that would make debugging the failure on ubuntu more plausible to debug

@sagudev sagudev force-pushed the extend_mode_norm branch from 94c02dd to efb8af0 Compare May 26, 2025 17:42
@sagudev
Copy link
Contributor Author

sagudev commented May 26, 2025

rebased!

@DJMcNab
Copy link
Member

DJMcNab commented May 29, 2025

I'm assuming that the failure on Ubuntu is because we're using a fallback CPU implementation of Vulkan, and therefore the precision is too high. What isn't at all clear to me is why the similar test from #971 passed on that version.

I think just marking the new tests as #[ignore] unconditionally is the least bad option to unblock us. We should however setup the usual skip_gpu_tests for them, so it doesn't fail on windows.

sagudev added 2 commits May 29, 2025 10:56
Signed-off-by: sagudev <[email protected]>
because CI uses SW which is more accurate then real GPUs

Signed-off-by: sagudev <[email protected]>
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small detail then we can land this. Thanks!

Signed-off-by: sagudev <[email protected]>
@DJMcNab DJMcNab added this pull request to the merge queue May 29, 2025
Merged via the queue into linebender:main with commit 7b32dc2 May 29, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Images do not roundtrip
2 participants